Skip to content

quic: split headers out from src/quic/stream.{h/cc} - #65863

Closed
jasnell wants to merge 1 commit into
nodejs:mainfrom
jasnell:jasnell/quic-split-headers-out
Closed

quic: split headers out from src/quic/stream.{h/cc}#65863
jasnell wants to merge 1 commit into
nodejs:mainfrom
jasnell:jasnell/quic-split-headers-out

Conversation

@jasnell

@jasnell jasnell commented Sep 6, 2026

Copy link
Copy Markdown
Member

As part of the effort to get better separation between the generalized QUIC streams and HTTP/3 streams, separate out header handling from the C++ class.

A smaller, incremental alternative approach to part of #63995

/cc @pimterry

@nodejs-github-bot

nodejs-github-bot commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/quic

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. labels Sep 6, 2026
@jasnell jasnell added the quic Issues and PRs related to the QUIC transport implementation. label Sep 6, 2026
@codecov

codecov Bot commented Sep 6, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

βœ… All modified and coverable lines are covered by tests.
βœ… Project coverage is 90.17%. Comparing base (1d0e5f1) to head (97e466a).
⚠️ Report is 30 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #65863      +/-   ##
==========================================
- Coverage   90.21%   90.17%   -0.04%     
==========================================
  Files         771      771              
  Lines      264911   264899      -12     
  Branches    50323    50311      -12     
==========================================
- Hits       238977   238880      -97     
- Misses      16917    16974      +57     
- Partials     9017     9045      +28
Files with missing lines Coverage Ξ”
lib/internal/quic/quic.js 100.00% <100.00%> (ΓΈ)
lib/internal/quic/state.js 100.00% <ΓΈ> (ΓΈ)

... and 33 files with indirect coverage changes

πŸš€ New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • πŸ“¦ JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@meixg meixg added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Sep 7, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Sep 7, 2026
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@jasnell
quic: split headers out from src/quic/stream.{h/cc}
97e466a 
As part of the effort to get better separation between
the generalized QUIC streams and HTTP/3 streams, separate
out header handling from the C++ class.

Signed-off-by: James M Snell <jasnell@gmail.com>
Assisted-by: Opencode
@jasnell
jasnell force-pushed the jasnell/quic-split-headers-out branch from 3c257cb to 97e466a Compare September 7, 2026 13:59
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@jasnell jasnell added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Sep 7, 2026
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@jasnell
jasnell requested a review from pimterry September 8, 2026 00:24
@nodejs-github-bot

nodejs-github-bot commented Sep 8, 2026 •
edited by jasnell
Loading

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/77179/ πŸ’š

pimterry
pimterry approved these changes Sep 8, 2026
View reviewed changes

@pimterry pimterry left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. In the other PR I do still want to take the internal separation further, but this is a very nice stepping stone, good to extract it (thanks!) and there's some nice standalone improvements in here I hadn't made in the larger PR at all πŸ‘

I'll rebase that again and pull those improvements here through once this lands.

@jasnell jasnell added commit-queue PRs queued for automated landing through the Commit Queue. and removed commit-queue PRs queued for automated landing through the Commit Queue. labels Sep 8, 2026
jasnell added a commit that referenced this pull request Sep 8, 2026
@jasnell
quic: split headers out from src/quic/stream.{h/cc}
450450c 
As part of the effort to get better separation between
the generalized QUIC streams and HTTP/3 streams, separate
out header handling from the C++ class.

Signed-off-by: James M Snell <jasnell@gmail.com>
Assisted-by: Opencode
PR-URL: #65863
Reviewed-By: Xuguang Mei <meixuguang@gmail.com>
Reviewed-By: Tim Perry <pimterry@gmail.com>
@jasnell

jasnell commented Sep 8, 2026

Copy link
Copy Markdown
Member Author

Landed in 450450c

@jasnell jasnell closed this Sep 8, 2026
@nodejs-github-bot nodejs-github-bot added the commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. label Sep 8, 2026
@nodejs-github-bot

nodejs-github-bot commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

Commit Queue failed

 * branch                  main       -> FETCH_HEAD
   ee354929da..450450c15a  main       -> origin/main
βœ”  origin/main is now up-to-date
main is out of sync with origin/main. Mismatched commits:
 - 450450c15a quic: split headers out from src/quic/stream.{h/cc}
--------------------------------------------------------------------------------
HEAD is now at 450450c15a quic: split headers out from src/quic/stream.{h/cc}
   βœ”  Reset to origin/main
- Downloading patch for 65863
fatal: couldn't find remote ref refs/pull/65863/merge

The pull request was removed from the Commit Queue and labeled commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. . After resolving the failure, remove that label and add commit-queue PRs queued for automated landing through the Commit Queue. to retry.

Full Commit Queue output 
- Loading data for nodejs/node/pull/65863
βœ”  Done loading data for nodejs/node/pull/65863
----------------------------------- PR info ------------------------------------
Title      quic: split headers out from src/quic/stream.{h/cc} (#65863)
Author     James M Snell <jasnell@gmail.com> (@jasnell)
Branch     jasnell:jasnell/quic-split-headers-out -> nodejs:main
Labels     c++, lib / src, author ready, needs-ci, quic
Commits    1
 - quic: split headers out from src/quic/stream.{h/cc}
Committers 1
 - James M Snell <jasnell@gmail.com>
PR-URL: https://github.com/nodejs/node/pull/65863
Reviewed-By: Xuguang Mei <meixuguang@gmail.com>
Reviewed-By: Tim Perry <pimterry@gmail.com>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/65863
Reviewed-By: Xuguang Mei <meixuguang@gmail.com>
Reviewed-By: Tim Perry <pimterry@gmail.com>
--------------------------------------------------------------------------------
   β„Ή  This PR was created on Sun, 06 Sep 2026 17:56:33 GMT
   βœ”  Approvals: 2
   βœ”  - Xuguang Mei (@meixg): https://github.com/nodejs/node/pull/65863#pullrequestreview-5127374667
   βœ”  - Tim Perry (@pimterry): https://github.com/nodejs/node/pull/65863#pullrequestreview-5144373845
   βœ”  Last GitHub CI successful
   β„Ή  Last Full PR CI on 2026-09-08T05:07:26Z: https://ci.nodejs.org/job/node-test-pull-request/77179/
- Querying data for job/node-test-pull-request/77179/
βœ”  Build data downloaded
   βœ”  Last Jenkins CI successful
--------------------------------------------------------------------------------
   βœ”  No git cherry-pick in progress
   βœ”  No git am in progress
   βœ”  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/main up to date...
From https://github.com/nodejs/node
 * branch                  main       -> FETCH_HEAD
   ee354929da..450450c15a  main       -> origin/main
βœ”  origin/main is now up-to-date
main is out of sync with origin/main. Mismatched commits:
 - 450450c15a quic: split headers out from src/quic/stream.{h/cc}
--------------------------------------------------------------------------------
HEAD is now at 450450c15a quic: split headers out from src/quic/stream.{h/cc}
   βœ”  Reset to origin/main
- Downloading patch for 65863
fatal: couldn't find remote ref refs/pull/65863/merge

View workflow run

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pimterry pimterry pimterry approved these changes
@meixg meixg meixg approved these changes
+1 more reviewer
@bjohansebas bjohansebas bjohansebas approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. quic Issues and PRs related to the QUIC transport implementation.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@jasnell @nodejs-github-bot @pimterry @meixg @bjohansebas